-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove requirement that geometry columns must not be a group field #234
Conversation
Good catch, though do we want to completely throw out the rule? Like we probably still don't want geometries nested in a group? And we could also just say that the only groupings allowed are the native geometries... I guess with this we'll probably need a 1.1.1? |
I think this might be a case where having more language in the spec is just going to create confusion. With the change I've proposed, our one requirement related to nesting/groups is that all geometry columns must be at the root of the schema. This implies (and is the same thing as saying) that geometry columns cannot be nested within other group fields. So I wouldn't add any additional words related to that. The encoding section already has a lot of language about how the encoding changes the structure of the geometry field, so I'm not sure more language is needed above. Basically, I think lots of language invites lots of differing interpretations. And with fewer words we risk fewer inconsistencies/misinterpretations/bugs etc.
Yeah, I think that makes sense. |
Cool, makes sense to me. Let's try to get this out soon, but ideally get a few more eyes on this PR. |
It's actually something we haven't discussed (or had to discuss) before, but do we want a new version number for something like this? Because it is not fixing something in the spec (in the sense of fixing something in implementations / files), but only fixing a wording in the spec. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change itself certainly looks good, thanks for catching that inconsistency!
Yeah, I think I could go either way - curious what others think. I agree it seems excessive to have implementations update just a version number when nothing really changes. On the other hand if this was written more formality like an OGC specification I do think this would be a change to the requirements / test cases / validators - indeed I think this came up since we didn't actually have any validators try to test 1.1 before we released. In the OGC version of the spec in requirement 2 it does say 'A geometry SHALL NOT be a group field or nested in a group.' So in that case it's not just wording, but it's changing one of the 'requirements'. At this point I probably lean slightly towards finding a way to not release a new version, since we have not actually adopted the OGC style of doing things, so there's a case to be made 'it's just wording'. But I'm not sure what we actually do then? Just merge this change to 1.2.0-dev and have 1.1.0 spec not have it? Update 1.1.0 tag and the published version on https://geoparquet.org/releases/v1.1.0/ to include this change? Something else? |
I guess one option would be to cut a 1.1.1 'release' on github (so there's a tag and the website gets updated), but keep the version the same? Could even call it 1.1.0.1 on github / spec? Or call it 1.1.0-update or something? |
In Python there's also the idea of "post releases" https://packaging.python.org/en/latest/specifications/version-specifiers/#post-releases
|
Oh nice, I like that. Good to use some precedent. |
Merging in, since no matter what we need this wording in. I'm going to try to run with the 'post release' idea and make a branch to propose it, as I'd like to get this wrapped up soon. |
@cholmes - with Semver this would be handled with "build metadata" following the version number - so We could make some changes to the way the website is built to only show the most recent release considering build metadata. Please don't follow the version naming convention from Python. |
Here is a summary of the latest tag and build metadata handling: #236 (comment) |
The new native geometry encodings from #189 are incompatible with the requirement that geometry columns must not be group fields. This branch suggests removing that requirement.
Fixes #233.